Adds the security layer to the VisaViewController, and updates tests.#36
Conversation
Also fixes a bug in the UserViewController for the /dashboard/admin endpoint.
📝 WalkthroughWalkthroughThe PR refactors authentication and authorization from a parameter-based approach to Spring Security annotations. All Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/templates/visa/dashboard.html (1)
125-126:⚠️ Potential issue | 🟠 MajorWire Sign Out to Spring Security’s real logout endpoint.
/visas/logoutis not the configured logout endpoint, whileSecurityConfigpermits/logout; this link will not reliably log users out. Use a POST form to/logoutwith CSRF.🔒 Proposed fix
- <a th:href="@{/visas/logout}" class="btn-new "> Sign Out - </a> + <form th:action="@{/logout}" method="post" style="margin: 0;"> + <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}" /> + <button type="submit" class="btn-new btn-signout">Sign Out</button> + </form>Based on learnings, the Sign Out control should use Spring Security’s standard
/logoutendpoint via POST with CSRF handling, or a dedicated controller that delegates toSecurityContextLogoutHandler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/dashboard.html` around lines 125 - 126, Replace the plain anchor link to "/visas/logout" with a POST-based logout form that posts to Spring Security’s standard "/logout" endpoint and includes the CSRF token; specifically, remove the <a> sign-out link and render a form (th:action="@{/logout}" method="post") containing a submit button and a hidden CSRF input using Thymeleaf expressions (use ${_csrf.parameterName} and ${_csrf.token}); alternatively, if you prefer controller-driven logout, wire the sign-out button to a dedicated controller method that invokes SecurityContextLogoutHandler to perform the logout and then redirects.src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
134-161:⚠️ Potential issue | 🟠 MajorCheck ownership before redisplaying invalid edit submissions.
A crafted invalid POST for another user’s visa skips
visaService.updateVisa(...), so the service-side ownership check never runs and line 160 can expose that visa’sstatusInformation. Load the visa and verify the applicant before handling validation errors.🛡️ Proposed fix
public String processUpdate( `@PathVariable` Long id, `@Valid` `@ModelAttribute` ("updateVisaDto") UpdateVisaDTO updateVisaDTO, BindingResult bindingResult, `@AuthenticationPrincipal` UserPrincipal principal, Model model) { + VisaDTO visa = visaService.findVisaDtoById(id); + if (!visa.applicantId().equals(principal.getUserId())) { + throw new UnauthorizedException("You can only edit your own applications."); + } + if (bindingResult.hasErrors()) { prepareApplyModel(principal.getUserId(), model); model.addAttribute("isEdit", true); - VisaDTO visa = visaService.findVisaDtoById(id); model.addAttribute("statusInformation", visa.statusInformation()); return "visa/edit-form"; } try { visaService.updateVisa(id, updateVisaDTO, principal.getUserId()); @@ prepareApplyModel(principal.getUserId(), model); model.addAttribute("isEdit", true); - VisaDTO visa = visaService.findVisaDtoById(id); model.addAttribute("statusInformation", visa.statusInformation()); return "visa/edit-form";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java` around lines 134 - 161, In processUpdate, fetch the target VisaDTO via visaService.findVisaDtoById(id) and verify the applicant matches principal.getUserId() before any early return or exception handling that redisplays the form; if the current user is not the owner, reject the request (throw AccessDeniedException or return a 403 view) instead of loading or exposing visa.statusInformation. Apply this check both at the start of the method (before the bindingResult.hasErrors() block) and in the catch block prior to calling visaService.findVisaDtoById(id) so ownership is always validated before redisplaying the edit form.
🧹 Nitpick comments (2)
src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
81-82: Remove stalecurrentUserIdparams from security-focused tests.The controller now ignores these params, so keeping them in requests makes the tests less effective at proving identity is principal-derived and not request-controlled.
🧹 Example cleanup
mockMvc.perform(post("/visas/apply") - .param("currentUserId", userId.toString()) .param("visaType", "TOURIST")Apply the same removal to the remaining visa requests that still send
currentUserId.Based on learnings,
currentUserIdshould be removed from the visa flows and replaced with authenticated-principal resolution.Also applies to: 121-122, 144-146, 212-215, 240-243, 269-272, 299-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 81 - 82, Remove the stale .param("currentUserId", ...) occurrences from the security-focused requests in VisaViewControllerTest (e.g., the mockMvc.perform(get("/visas/dashboard")...) call and the other listed visa request calls) so tests no longer rely on request-controlled identity; instead ensure the request identity is provided via authenticated principal resolution (use the existing security test helpers to set the principal, e.g., the SecurityMockMvcRequestPostProcessors or your test user setup) and update the assertions to validate principal-derived behavior.src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
150-160: The change from"UNASSIGNED"to"SUBMITTED"correctly fixes the bug.
VisaStatushas noUNASSIGNEDenum value, so the old code would have thrown anIllegalArgumentExceptioninfindByStatus(), resulting in an empty unassigned cases list. The fix to use"SUBMITTED"(which exists in the enum) is correct—it represents newly submitted, unassigned cases.Optional: Consider using the enum constant instead of a string literal:
- List<VisaDTO> unassignedCases = visaService.findVisaByStatus("SUBMITTED"); + List<VisaDTO> unassignedCases = visaService.findVisaByStatus(VisaStatus.SUBMITTED.name());This eliminates the string literal, though the current implementation already validates and throws
IllegalArgumentExceptionon invalid status values, so the risk is low.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 150 - 160, The adminDashboard method currently passes the string "SUBMITTED" to visaService.findVisaByStatus; replace this string literal with the VisaStatus enum constant to avoid brittle string usage—call the service with VisaStatus.SUBMITTED (or convert enum to the required method parameter type) in adminDashboard (which uses visaService.findVisaByStatus and visaService.findVisasByHandlerId) so the code relies on the enum instead of a raw string and prevents future typos or invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Line 39: The security rule allowing any authenticated user for
"/api/comments/**" is too permissive; tighten it in SecurityConfig by replacing
.requestMatchers("/api/comments/**").authenticated() with a role-restricted
matcher such as .requestMatchers("/api/comments/**").hasRole("USER") (or another
appropriate authority used by your app) so only intended roles can access
comment endpoints, and then update CommentController (and the service it calls,
e.g., createComment) to stop trusting CreateCommentDTO.authorId(): obtain the
author from the authenticated principal (via `@AuthenticationPrincipal` or
SecurityContextHolder) and enforce visa-access checks there before creating or
returning comments.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 474-484: The tests currently mutate SecurityContextHolder in
createMockUser() by building a UserPrincipal and TestingAuthenticationToken;
instead refactor createMockUser(Long id, UserAuthorization role) to return a
plain UserDTO (no SecurityContextHolder or UserPrincipal creation) and stop
calling SecurityContextHolder.getContext().setAuthentication(...) in that
helper; for each MockMvc request attach the authentication scoped to that
request with .with(authentication(new TestingAuthenticationToken(new
UserPrincipal(userEntityOrDTO), "password", authorities))) (or use Spring
Security's RequestPostProcessor), remove all stale .param("currentUserId", ...)
from requests, and keep using `@WithMockUser` only where appropriate so
`@AuthenticationPrincipal` and `@PreAuthorize` resolve the correct per-request
principal.
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 134-161: In processUpdate, fetch the target VisaDTO via
visaService.findVisaDtoById(id) and verify the applicant matches
principal.getUserId() before any early return or exception handling that
redisplays the form; if the current user is not the owner, reject the request
(throw AccessDeniedException or return a 403 view) instead of loading or
exposing visa.statusInformation. Apply this check both at the start of the
method (before the bindingResult.hasErrors() block) and in the catch block prior
to calling visaService.findVisaDtoById(id) so ownership is always validated
before redisplaying the edit form.
In `@src/main/resources/templates/visa/dashboard.html`:
- Around line 125-126: Replace the plain anchor link to "/visas/logout" with a
POST-based logout form that posts to Spring Security’s standard "/logout"
endpoint and includes the CSRF token; specifically, remove the <a> sign-out link
and render a form (th:action="@{/logout}" method="post") containing a submit
button and a hidden CSRF input using Thymeleaf expressions (use
${_csrf.parameterName} and ${_csrf.token}); alternatively, if you prefer
controller-driven logout, wire the sign-out button to a dedicated controller
method that invokes SecurityContextLogoutHandler to perform the logout and then
redirects.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 150-160: The adminDashboard method currently passes the string
"SUBMITTED" to visaService.findVisaByStatus; replace this string literal with
the VisaStatus enum constant to avoid brittle string usage—call the service with
VisaStatus.SUBMITTED (or convert enum to the required method parameter type) in
adminDashboard (which uses visaService.findVisaByStatus and
visaService.findVisasByHandlerId) so the code relies on the enum instead of a
raw string and prevents future typos or invalid values.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 81-82: Remove the stale .param("currentUserId", ...) occurrences
from the security-focused requests in VisaViewControllerTest (e.g., the
mockMvc.perform(get("/visas/dashboard")...) call and the other listed visa
request calls) so tests no longer rely on request-controlled identity; instead
ensure the request identity is provided via authenticated principal resolution
(use the existing security test helpers to set the principal, e.g., the
SecurityMockMvcRequestPostProcessors or your test user setup) and update the
assertions to validate principal-derived behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3858d0b-e92f-4997-8065-e25c4049880b
📒 Files selected for processing (10)
src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/resources/templates/visa/apply-form.htmlsrc/main/resources/templates/visa/dashboard.htmlsrc/main/resources/templates/visa/details.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
| .requestMatchers("/dashboard").authenticated() | ||
| .requestMatchers("/profile/**").authenticated() | ||
| .requestMatchers("/visas/**").authenticated() | ||
| .requestMatchers("/api/comments/**").authenticated() |
There was a problem hiding this comment.
Don’t loosen the comments API before fixing comment authorization.
/api/comments/** is now available to any authenticated user, but CommentController has no role/access checks and the existing comment creation path still trusts CreateCommentDTO.authorId(). Until comments resolve author and visa access from the principal, keep this endpoint stricter or add controller/service-level authorization.
🛡️ Conservative fix until comment auth is implemented
- .requestMatchers("/api/comments/**").authenticated()
+ .requestMatchers("/api/comments/**").hasRole("SYSADMIN")Based on learnings, CreateCommentDTO.authorId() is currently trusted directly and should be replaced with the authenticated principal once Spring Security is integrated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .requestMatchers("/api/comments/**").authenticated() | |
| .requestMatchers("/api/comments/**").hasRole("SYSADMIN") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`
at line 39, The security rule allowing any authenticated user for
"/api/comments/**" is too permissive; tighten it in SecurityConfig by replacing
.requestMatchers("/api/comments/**").authenticated() with a role-restricted
matcher such as .requestMatchers("/api/comments/**").hasRole("USER") (or another
appropriate authority used by your app) so only intended roles can access
comment endpoints, and then update CommentController (and the service it calls,
e.g., createComment) to stop trusting CreateCommentDTO.authorId(): obtain the
author from the authenticated principal (via `@AuthenticationPrincipal` or
SecurityContextHolder) and enforce visa-access checks there before creating or
returning comments.
| // Helper methods | ||
| private UserDTO createMockUser(Long id, UserAuthorization role) { | ||
| User testUser = new User(); | ||
| testUser.setId(id); | ||
| testUser.setUsername("test@test.com"); | ||
| testUser.setEmail("test@test.com"); | ||
| testUser.setPassword("password"); | ||
| testUser.setUserAuthorization(role); | ||
| UserPrincipal principal = new UserPrincipal(testUser); | ||
| Authentication authentication = new TestingAuthenticationToken(principal, "password", principal.getAuthorities()); | ||
| SecurityContextHolder.getContext().setAuthentication(authentication); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether this test mixes `@WithMockUser` with manual SecurityContextHolder mutation
# instead of request-scoped MockMvc authentication.
# Expected result before the fix: matches for `@WithMockUser` and SecurityContextHolder.setAuthentication,
# with no/limited `.with(authentication(...))` usage on requests.
rg -n -C2 '@WithMockUser|SecurityContextHolder\.getContext\(\)\.setAuthentication|\.with\(authentication\()' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.javaRepository: ithsjava25/visa-case-management-system
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Fixed search for authentication patterns in the test file
# Using separate patterns to avoid regex parsing errors
echo "=== Searching for `@WithMockUser` ==="
rg -n '@WithMockUser' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
echo -e "\n=== Searching for SecurityContextHolder.getContext().setAuthentication ==="
rg -n 'SecurityContextHolder.*setAuthentication' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
echo -e "\n=== Searching for .with(authentication pattern ==="
rg -n '\.with\(authentication' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
echo -e "\n=== Searching for param currentUserId (stale params) ==="
rg -n 'param.*currentUserId' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.javaRepository: ithsjava25/visa-case-management-system
Length of output: 1219
🏁 Script executed:
#!/bin/bash
# Read the area around the createMockUser helper and check for its usage
echo "=== Reading VisaViewControllerTest around createMockUser (lines 470-490) ==="
sed -n '470,490p' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
echo -e "\n=== Searching for calls to createMockUser ==="
rg -n 'createMockUser' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
echo -e "\n=== Read first test method to see overall pattern (around line 53-90) ==="
sed -n '53,90p' src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.javaRepository: ithsjava25/visa-case-management-system
Length of output: 4385
Scope the custom UserPrincipal authentication to each MockMvc request instead of mutating SecurityContextHolder.
The test file mixes @WithMockUser with manual SecurityContextHolder.getContext().setAuthentication() calls across all 18 test methods. When a controller method uses @AuthenticationPrincipal UserPrincipal, it may resolve to the wrong principal or fail, undermining @PreAuthorize coverage. Additionally, stale .param("currentUserId", ...) parameters in requests (lines 82, 122, 145, 214, 242, 271, 301) contradict the expected removal of currentUserId from controller method signatures.
Refactor createMockUser() to return a plain UserDTO without mutating SecurityContextHolder, then attach authentication per request using .with(authentication(...)):
Recommended refactor
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication;
+
+ private Authentication createAuthentication(Long id, UserAuthorization role) {
+ User testUser = new User();
+ testUser.setId(id);
+ testUser.setUsername("test@test.com");
+ testUser.setEmail("test@test.com");
+ testUser.setPassword("password");
+ testUser.setUserAuthorization(role);
+ UserPrincipal principal = new UserPrincipal(testUser);
+ return new TestingAuthenticationToken(principal, "password", principal.getAuthorities());
+ }Apply per request:
- mockMvc.perform(get("/visas/dashboard"))
+ mockMvc.perform(get("/visas/dashboard")
+ .with(authentication(createAuthentication(adminId, UserAuthorization.ADMIN))))Also remove stale .param("currentUserId", ...) from all requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`
around lines 474 - 484, The tests currently mutate SecurityContextHolder in
createMockUser() by building a UserPrincipal and TestingAuthenticationToken;
instead refactor createMockUser(Long id, UserAuthorization role) to return a
plain UserDTO (no SecurityContextHolder or UserPrincipal creation) and stop
calling SecurityContextHolder.getContext().setAuthentication(...) in that
helper; for each MockMvc request attach the authentication scoped to that
request with .with(authentication(new TestingAuthenticationToken(new
UserPrincipal(userEntityOrDTO), "password", authorities))) (or use Spring
Security's RequestPostProcessor), remove all stale .param("currentUserId", ...)
from requests, and keep using `@WithMockUser` only where appropriate so
`@AuthenticationPrincipal` and `@PreAuthorize` resolve the correct per-request
principal.
eeebbaandersson
left a comment
There was a problem hiding this comment.
Ser fint ut, snyggt jobbat!
Also fixes a bug in the UserViewController for the /dashboard/admin endpoint.
Summary by CodeRabbit
New Features
Refactor
Tests